fix: only merge token transfers when approval status matches by Yashraj-Jangra · Pull Request #2655 · hiero-ledger/hiero-sdk-python

Yashraj-Jangra · 2026-09-11T12:41:58Z

Description:

In AbstractTokenTransferTransaction._add_token_transfer, token transfers for the same (token_id, account_id) pair were being merged without checking is_approved. This caused approved transfers to get merged into standard transfers (or vice versa), silently clearing the approval flag depending on the order they were added.

Since spending an allowance on behalf of an owner and transferring an account's own balance are two distinct actions, they shouldn't be combined into a single entry. This updates the accumulation logic so transfers only merge when both the account ID and approval status match.

Only merge transfers in _add_token_transfer when both account_id and is_approved match
Keep previously set expected_decimals when merging if the incoming transfer passes expected_decimals=None
Update test_approved_token_transfer_accumulation to assert that normal and approved transfers stay separate
Add unit tests covering separation, accumulation of matching approved transfers, non-decimal helper, and decimal preservation
Update test_integration_transfer_transaction_approved_token_transfer to model the allowance flow correctly (approved debit from owner, credit to recipient) so consensus nodes don't reject repeated accounts in the same transfer list

Related issue(s):

Fixes #2253

Notes for reviewer:

All 55 tests in transfer_transaction_test.py pass with full line coverage on the modified logic. Full unit test suite (3,025 tests) and pre-commit checks (ruff, bandit) pass clean.

Checklist

Documented (Code comments, README, etc.)
Tested (unit, integration, etc.)

Copilot

🟡 Changes recommended

Fix mixed-order decimal serialization and rename the misleading unit test.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates token transfer accumulation to distinguish approval status, preserve decimal metadata, and correct allowance-flow integration coverage.

Changes:

Separates normal and approved transfers.
Preserves decimal metadata when merging.
Expands unit tests and corrects integration coverage.

File summaries

File	Summary
`tests/unit/transfer_transaction_test.py`	Adds approval and decimal-preservation coverage; one test should be renamed for clarity.
`tests/integration/transfer_transaction_e2e_test.py`	Corrects the approved allowance transfer flow.
`src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.py`	Updates merging logic; mixed-order decimal metadata still needs protobuf serialization coverage and correction.

Review details

Suppressed comments (1)

tests/unit/transfer_transaction_test.py:458

test_approved_token_transfer_accumulation no longer tests accumulation; it now verifies separation, while test_same_approved_transfers_accumulate covers accumulation below. Rename this test so its name matches the behavior it asserts and failures remain diagnosable.

    """Test that approved token transfers are stored as separate entries from normal ones."""

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+            if transfer.account_id == account_id and transfer.is_approved == is_approved:
                transfer.amount += amount
-                transfer.expected_decimals = expected_decimals
+                if expected_decimals is not None:
+                    transfer.expected_decimals = expected_decimals


coderabbitai · 2026-09-11T12:50:28Z

Walkthrough

The transfer merge logic now keeps regular and approved transfers separate, merges transfers only when approval status matches, and preserves expected_decimals when omitted. Unit and integration tests validate the updated behavior.

Changes

Token transfer merge behavior

Layer / File(s)	Summary
Approval-aware transfer merging `src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.py`, `tests/unit/transfer_transaction_test.py`	`_add_token_transfer` separates regular and approved transfers, accumulates matching transfers, preserves omitted `expected_decimals`, and validates `is_approved`.
Approved allowance transfer validation `tests/integration/transfer_transaction_e2e_test.py`	The integration test uses an approved debit from `env.operator_id` and a regular credit to `account.id`.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Severity of issue fixed: Medium

Merge Risk: 🟡 Moderate · up to e4208

Mixed regular and approved transfers can bypass requested token-decimal validation, while an incorrect allowance debit or credit would not be detected by the integration test. These should be addressed before merge.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: token transfers merge only when their approval status matches.
Description check	✅ Passed	The description directly explains the approval-status merge fix, expected-decimals behavior, tests, and integration-flow updates.
Linked Issues check	✅ Passed	For `#2253`, `_add_token_transfer` now merges only when `account_id` and `is_approved` both match. Normal and approved transfers for the same account remain separate, so approval state is not lost. Tran…
Out of Scope Changes check	✅ Passed	The source change directly fixes `_add_token_transfer` behavior for `#2253`. The unit tests verify normal and approved transfer separation, same-status accumulation, decimal preservation, and type valid…
Docstring Coverage	✅ Passed	Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tests/integration/transfer_transaction_e2e_test.py (1)
385-385: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert resulting token balances.

The receipt assertion does not verify the changed allowance transfer result. Query both accounts after execution. Assert that env.operator_id has 500 units and account.id has 500 units for token_id.
Proposed test addition
+    operator_balance = CryptoGetAccountBalanceQuery(env.operator_id).execute(env.client)
+    account_balance = CryptoGetAccountBalanceQuery(account.id).execute(env.client)
+
+    assert operator_balance.token_balances[token_id] == 500
+    assert account_balance.token_balances[token_id] == 500
As per path instructions, “Tests should assert observable network behavior, not just SUCCESS.”

Source: Path instructions

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: a0effce5-131d-4701-b968-f5b565494e1c

📥 Commits

Reviewing files that changed from the base of the PR and between 19e7f37 and 4448628.

📒 Files selected for processing (3)

src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.py
tests/integration/transfer_transaction_e2e_test.py
tests/unit/transfer_transaction_test.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

coderabbitai · 2026-09-11T12:50:31Z

+            if transfer.account_id == account_id and transfer.is_approved == is_approved:
                transfer.amount += amount
-                transfer.expected_decimals = expected_decimals
+                if expected_decimals is not None:
+                    transfer.expected_decimals = expected_decimals


🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Keep expected_decimals token-scoped for approval-separated transfers.

Proto field: TokenTransferList.expected_decimals (field 4). Issue type: Asymmetric round-trip. The schema defines this value once per token transfer list, not once per account amount. (github.com)

If a regular transfer without decimals is added first and an approved transfer with decimals is added second, Line 164 prevents a merge. build_token_transfers() then serializes the first entry's None value and drops the caller's expected decimals. The transaction can skip the requested decimal validation.

src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.py#L164-L167: propagate an explicit decimal value to the token's existing entries, and inherit the token decimal value for a new entry when the caller passes None.

tests/unit/transfer_transaction_test.py#L548-L583: build a transaction with a normal no-decimal transfer followed by an approved decimal transfer, then assert tokenTransfers[0].expected_decimals.value.

Proposed implementation direction

+ transfers = self.token_transfers[token_id] + if expected_decimals is not None: + for existing_transfer in transfers: + existing_transfer.expected_decimals = expected_decimals + elif transfers: + expected_decimals = transfers[0].expected_decimals + - for transfer in self.token_transfers[token_id]: + for transfer in transfers: if transfer.account_id == account_id and transfer.is_approved == is_approved: transfer.amount += amount - if expected_decimals is not None: - transfer.expected_decimals = expected_decimals + transfer.expected_decimals = expected_decimals return - self.token_transfers[token_id].append( + transfers.append( TokenTransfer(token_id, account_id, amount, expected_decimals, is_approved) )

As per path instructions, protobuf fields must match their schema semantics and unit tests must cover edge cases.

📍 Affects 2 files

src/hiero_sdk_python/tokens/abstract_token_transfer_transaction.py#L164-L167 (this comment)

tests/unit/transfer_transaction_test.py#L548-L583

Source: Path instructions

Yashraj-Jangra · 2026-09-11T12:51:24Z

@MonaaEid Please review this PR

Signed-off-by: Yashraj Jangra <84060578+Yashraj-Jangra@users.noreply.github.com>

Yashraj-Jangra requested review from a team as code owners September 11, 2026 12:41

Copilot AI lite review requested due to automatic review settings September 11, 2026 12:41

Copilot started reviewing on behalf of Yashraj-Jangra September 11, 2026 12:42 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

coderabbitai Bot reviewed Sep 11, 2026

View reviewed changes

github-actions Bot added open to community review PR is open for community review and feedback queue:junior-committer PR awaiting initial quality review labels Sep 11, 2026

fix: only merge token transfers when approval status matches

e4208bb

Signed-off-by: Yashraj Jangra <84060578+Yashraj-Jangra@users.noreply.github.com>

Yashraj-Jangra force-pushed the fix/token-transfer-approved-accumulation branch from 4448628 to e4208bb Compare September 11, 2026 21:15

Navigation Menu

Uh oh!

fix: only merge token transfers when approval status matches - #2655

fix: only merge token transfers when approval status matches#2655
Yashraj-Jangra wants to merge 1 commit into
hiero-ledger:mainfrom
Yashraj-Jangra:fix/token-transfer-approved-accumulation

Yashraj-Jangra commented Sep 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

coderabbitai Bot commented Sep 11, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Sep 11, 2026

Uh oh!

Yashraj-Jangra commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Yashraj-Jangra commented Sep 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

coderabbitai Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

Yashraj-Jangra commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Sep 11, 2026 •

edited

Loading